Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GH-15824 mb_detect_encoding() invalid "UTF8" #15829

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

youkidearitai
Copy link
Contributor

@youkidearitai youkidearitai commented Sep 10, 2024

I fixed from strcasecmp to strncasecmp.
However, strncasecmp is specify size to 3rd parameter.
Hence, Add check strlen to mime and aliases.

@@ -332,6 +332,10 @@ const mbfl_encoding *mbfl_name2encoding_ex(const char *name, size_t name_len)
}
#endif

if (name_len == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check shouldn't be necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Removed.

mbstring
--FILE--
<?php
echo mb_detect_encoding('abc', 'UTF8, ASCII');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add more test cases such that both mime names and aliases are tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

ext/mbstring/libmbfl/mbfl/mbfl_encoding.c Outdated Show resolved Hide resolved
@@ -349,7 +353,7 @@ const mbfl_encoding *mbfl_name2encoding_ex(const char *name, size_t name_len)
/* search MIME charset name */
for (encoding = mbfl_encoding_ptr_list; *encoding; encoding++) {
if ((*encoding)->mime_name) {
if (strcasecmp((*encoding)->mime_name, name) == 0) {
if (strcasecmp((*encoding)->mime_name, name) == 0 && strlen((*encoding)->mime_name) == name_len) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use strncasecmp and the same == '\0' check that I suggested below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for pretty code. Fixed.

@@ -349,7 +349,7 @@ const mbfl_encoding *mbfl_name2encoding_ex(const char *name, size_t name_len)
/* search MIME charset name */
for (encoding = mbfl_encoding_ptr_list; *encoding; encoding++) {
if ((*encoding)->mime_name) {
if (strcasecmp((*encoding)->mime_name, name) == 0) {
if (strcasecmp((*encoding)->mime_name, name) == 0 && (*encoding)->mime_name[name_len] == '\0') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still need strncasecmp here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thanks.

I fixed from strcasecmp to strncasecmp.
However, strncasecmp is specify size to php#3 parameter.
Hence, Add check length to mime and aliases.

Co-authored-by: Niels Dossche <[email protected]>
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@youkidearitai youkidearitai merged commit dc5f3b9 into php:master Sep 11, 2024
10 checks passed
@youkidearitai youkidearitai deleted the fix-gh-15824 branch September 11, 2024 00:40
@alexdowad
Copy link
Contributor

@youkidearitai Indeed, thanks. It seems you are a committer for php-src now? If so, do you want to re-apply 0bcc1e613bc?

@youkidearitai
Copy link
Contributor Author

@alexdowad Thank you for confirmed. Add my account for CODEOWNERS file is already applied in #14744 . Thank you.

@alexdowad
Copy link
Contributor

@youkidearitai Thanks very much 👍 In future, it will be appreciated if you can CC me on such changes.

As a suggestion, if you want to list yourself as a 2nd mbstring maintainer in EXTENSIONS, I have no objection to that.

@youkidearitai
Copy link
Contributor Author

youkidearitai commented Sep 11, 2024

@alexdowad

it will be appreciated if you can CC me on such changes.

Okay. I will do in next time.

As a suggestion, if you want to list yourself as a 2nd mbstring maintainer in EXTENSIONS, I have no objection to that.

Thank you. I will add myself for EXTENSIONS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ValueError: mb_detect_encoding(): Argument #2 ($encodings) contains invalid encoding "UTF8"
3 participants